-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace flake8 and isort with ruff #6128
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6128 +/- ##
==========================================
- Coverage 82.42% 82.41% -0.01%
==========================================
Files 350 350
Lines 21457 21423 -34
Branches 839 839
==========================================
- Hits 17685 17656 -29
+ Misses 3474 3469 -5
Partials 298 298
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
26b6c17
to
a4968dd
Compare
I would like not to mix the shift from isort+flake8 to ruff with the actual fixing of the issues. Can we have a PR that only converts the style tooling (adding more exceptions if needed), and then fixing and removing exceptions in future PRs |
Not sure I agree since this makes it clear that the changes were done to please the linter, but I'll see what I can do. |
@@ -167,8 +167,8 @@ def valid_num_iterations(user_input: str) -> str: | |||
def attemp_int_conversion(val: str) -> int: | |||
try: | |||
return int(val) | |||
except ValueError: | |||
raise ArgumentTypeError(f"{val} is not a valid integer") | |||
except ValueError as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an automated fix. Is it given that we always want to reraise exceptions and keep all the details (it probably often is). When seen by users, more details can make them blind.
If this is a new requirement imposed by ruff, it would like to have a global exception for it in ruff-configuration, and then we can think about solving it in a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really automated, I just did not see any reason to hide the context and yes, it makes ruff
happy.
Also, I'm not really sure what the difference would be between the two in the specific function.
I would guess that they do the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good improvement!
@@ -266,7 +266,7 @@ def get_documentation_for_jobs(self) -> Dict[str, Any]: | |||
self.hook.installable_jobs(), include_plugin_data=True | |||
).items() | |||
} | |||
for k in job_docs.keys(): | |||
for k in job_docs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what k
is? Not introducing it, I know, but if you know, perhaps rename it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what it is unfortunately.
@@ -35,9 +35,7 @@ def test_using_qt_model_tester(qtmodeltester, full_snapshot): | |||
model.setSourceModel(source_model) | |||
|
|||
reporting_mode = qt_api.QtTest.QAbstractItemModelTester.FailureReportingMode.Warning | |||
tester = qt_api.QtTest.QAbstractItemModelTester( # noqa, prevent GC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps relevant to keep the prevent GC
part of the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -56,9 +54,7 @@ def test_changes(full_snapshot): | |||
model.setSourceModel(source_model) | |||
|
|||
reporting_mode = qt_api.QtTest.QAbstractItemModelTester.FailureReportingMode.Warning | |||
tester = qt_api.QtTest.QAbstractItemModelTester( # noqa, prevent GC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Flake8 does not support pyproject.toml out-of-the box and ruff is faster.
Pre review checklist
Ground Rules),
and changes to existing code have good test coverage.
Pre merge checklist